Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[feat, UX] Add multiswipe recorder #4644

Merged
merged 2 commits into from Feb 22, 2019
Merged

[feat, UX] Add multiswipe recorder #4644

merged 2 commits into from Feb 22, 2019

Conversation

Frenzie
Copy link
Member

@Frenzie Frenzie commented Feb 22, 2019


@Frenzie Frenzie added this to the 2019.03 milestone Feb 22, 2019
@Frenzie Frenzie requested a review from poire-z February 22, 2019 18:21
poire-z
poire-z approved these changes Feb 22, 2019
View reviewed changes
Copy link
Contributor

@poire-z poire-z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, lgtm.
(may be remove the blank lines 95 & 97)

@Frenzie Frenzie merged commit 2e255a1 into koreader:master Feb 22, 2019
@Frenzie Frenzie deleted the multiswipe-recorder branch February 22, 2019 19:59
Frenzie added a commit to Frenzie/koreader that referenced this pull request Feb 22, 2019
@Frenzie
[fix] ReaderGesture: don't crash without custom gestures
2455aa1 
Silly oversight in koreader#4644.
@Frenzie Frenzie mentioned this pull request Feb 22, 2019
Merged
Frenzie added a commit that referenced this pull request Feb 22, 2019
@Frenzie
[fix] ReaderGesture: don't crash without custom gestures (#4645)
9217481 
Silly oversight in #4644.

Also remove unused util because apparently CircleCI didn't run on the other PR when it should've.
@NiLuJe
Copy link
Member

NiLuJe commented Feb 22, 2019

Obligatory Konami code reference. :D

@Frenzie
Copy link
Member Author

Frenzie commented Feb 23, 2019

Haha, it does look a bit like it! :D

@poire-z
Copy link
Contributor

poire-z commented Feb 24, 2019

(Many multiswipe PRs, don't really know where to put my comments :) so putting them in the related PR even if one single place would may be better).

When recording/adding a new gesture, one needs to restart koreader to see it in the menu and be able to assign some action to it.
Not really bothering, but might need some InfoMessage inviting to do so if that can't be workarounded.

@Frenzie
Copy link
Member Author

Frenzie commented Feb 24, 2019

It's easy enough to work around, but I was hoping there was something more proper in place, see #4651.

diff --git a/frontend/apps/reader/modules/readergesture.lua b/frontend/apps/reader/modules/readergesture.lua
index 45f75eff..a180b776 100644
--- a/frontend/apps/reader/modules/readergesture.lua
+++ b/frontend/apps/reader/modules/readergesture.lua
@@ -55,6 +55,15 @@ function ReaderGesture:initGesture()
     G_reader_settings:saveSetting(self.ges_mode, gesture_manager)
 end
 
+function ReaderGesture:genMultiswipeSubmenu()
+    return {
+        text = _("Multiswipe"),
+        sub_item_table = self:buildMultiswipeMenu(),
+        enabled_func = function() return self.multiswipes_enabled end,
+        separator = true,
+    }
+end
+
 function ReaderGesture:addToMainMenu(menu_items)
     menu_items.gesture = {
         text = _("Gesture manager"),
@@ -71,7 +80,7 @@ function ReaderGesture:addToMainMenu(menu_items)
             {
                 text = _("Multiswipe recorder"),
                 enabled_func = function() return self.multiswipes_enabled end,
-                callback = function()
+                callback = function(touchmenu_instance)
                     local multiswipe_recorder
                     multiswipe_recorder = InputDialog:new{
                         title = _("Multiswipe recorder"),
@@ -92,6 +101,8 @@ function ReaderGesture:addToMainMenu(menu_items)
                                         if not recorded_multiswipe then return end
                                         logger.dbg("Multiswipe recorder detected:", recorded_multiswipe)
                                         custom_multiswipes:addTableItem("multiswipes", recorded_multiswipe)
+                                        -- TODO implement some nicer method in TouchMenu than this ugly hack for updating the menu
+                                        touchmenu_instance.item_table[3] = self:genMultiswipeSubmenu()
                                         UIManager:close(multiswipe_recorder)
                                     end,
                                 },

even if one single place would may be better

I think just opening a new issue would be easier to keep track of. :-)

Frenzie added a commit to Frenzie/koreader that referenced this pull request Feb 24, 2019
@Frenzie
[UX] Gesture manager: delete custom gestures and update menu
b1510c0 
Also extend the default set of gestures to include all of them, and add east south west north for refresh. (I've always thought small diagonal swipe to be rather awkward to trigger.)

See koreader#4644 (comment) and koreader#4651 regarding the menu update.
Frenzie added a commit to Frenzie/koreader that referenced this pull request Feb 24, 2019
@Frenzie
[UX] Gesture manager: delete custom gestures and update menu
9f1b45c 
Include a safety check to prevent rerecording an existing gesture.

Also extend the default set of gestures to include all of them, and add east south west north for refresh. (I've always thought small diagonal swipe to be rather awkward to trigger.)

See koreader#4644 (comment) and koreader#4651 regarding the menu update.
@Frenzie Frenzie mentioned this pull request Feb 24, 2019
Merged
Frenzie added a commit that referenced this pull request Feb 24, 2019
@Frenzie
[UX] Gesture manager: delete custom gestures and update menu (#4652)
91b83c5 
Include a safety check to prevent rerecording an existing gesture.

Also extend the default set of gestures to include all of them, and add east south west north for refresh. (I've always thought small diagonal swipe to be rather awkward to trigger.)

See #4644 (comment) and #4651 regarding the menu update.
mwoz123 pushed a commit to mwoz123/koreader that referenced this pull request Mar 29, 2020
@Frenzie @mwoz123
[feat, UX] Add multiswipe recorder (koreader#4644)
b5f499d
mwoz123 pushed a commit to mwoz123/koreader that referenced this pull request Mar 29, 2020
@Frenzie @mwoz123
[fix] ReaderGesture: don't crash without custom gestures (koreader#4645)
4d90b25 
Silly oversight in koreader#4644.

Also remove unused util because apparently CircleCI didn't run on the other PR when it should've.
mwoz123 pushed a commit to mwoz123/koreader that referenced this pull request Mar 29, 2020
@Frenzie @mwoz123
[UX] Gesture manager: delete custom gestures and update menu (koreade…
0c780cc 
…r#4652)

Include a safety check to prevent rerecording an existing gesture.

Also extend the default set of gestures to include all of them, and add east south west north for refresh. (I've always thought small diagonal swipe to be rather awkward to trigger.)

See koreader#4644 (comment) and koreader#4651 regarding the menu update.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poire-z poire-z poire-z approved these changes

Assignees
No one assigned
Labels
enhancement UX
Projects
None yet
Milestone
2019.03
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Frenzie @NiLuJe @poire-z